Update plotSpectra environment to allow addFragments returned list #348

guideflandre · 2025-01-22T16:14:11Z

This PR is linked to this PSMatch issue and the discussed topics in #346

The motivation for this PR:

addFragments used to throw an error when multiple spectra where called
The new variable_modifications parameter necessitated a new addFragments function to distinguish annotations

The solution:
addFragments now returns a list of named elements (peptide sequence that includes modifications) containing a character() vector with the annotations. Each element of the list has an attribute linking the annotations to the spectrum it belonged to. In Spectra, the changes are located at the level of the presence of labels. It is assumed that most often than not, addFragments is called for labels. A quick check for variable modifications is also added (as plotSpectra doesn't support them yet). All three functions (plotSpectra, plotSpectraMirror and plotSpectraOverlay now support the version of addFragments in this PR).

This PR can only be accepted if the corresponding PR in PSMatch is accepted !! Otherwise, plotSpectra won't work.

jorainer

Thanks for the PR! I have however some questions.

Spectra.Rproj

R/plotting-functions.R

jorainer · 2025-01-23T12:20:04Z

General comment: I don't know your specific use case, but would it not be possible to add whatever information/annotation you have or calculate directly to the Spectra object (as peaks or spectra variables) before plotting and then have a labels() function that can extract that information (or that even calculates it on the fly)? that was also the initial idea of accepting a function with parameter labels.

guideflandre · 2025-01-23T13:02:53Z

General comment: I don't know your specific use case, but would it not be possible to add whatever information/annotation you have or calculate directly to the Spectra object (as peaks or spectra variables) before plotting and then have a labels() function that can extract that information (or that even calculates it on the fly)? that was also the initial idea of accepting a function with parameter labels.

I believe we could if the number of annotations don't exceed length(x). In my use case, with variable modifications, I would have multiple annotations/spectrum which is not ideal.

guideflandre · 2025-01-23T14:01:31Z

A discussion with Laurent led to a potential different idea: the creation of a package specifically for plotting spectra. I understand my use cases might not fit in well within the plotSpectra environment, and it might need its separate function/package. I believe this should be an option/discussed too.

lgatto · 2025-03-06T13:53:33Z

@jorainer - we need to move on with this issue to be able to merge the new PSMatch functionality. Do we need another discussion.

… plotting functions

sgibb

I am not aware of possible side effects of changing labels = character() to labels = list(). Did you try to rebuild the vignettes and reverse dependencies?

R/plotting-functions.R

sgibb · 2025-04-03T15:22:03Z

R/plotting-functions.R

@@ -319,7 +345,7 @@ setMethod(
            xlim[1L] <- xlim[1L] - wdths
            xlim[2L] <- xlim[2L] + wdths
            plot.window(xlim = xlim, ylim = ylim, ...)
-        }
+        } else {labels <- NULL}


Do we need this? Isn't the if (length(labels)) statement already enough?

Maybe, we should change the default to label = list() instead of the current label = character().

Actually we do need it. If labels is an empty list, when calling .plot_single_spectrum, an error occurs: the subscript is out of bounds (labels[[i]] of an empty list). If labels is NULL, this error does not occur

sgibb · 2025-04-03T15:27:00Z

R/plotting-functions.R

    for (i in seq_len(nsp))
        .plot_single_spectrum(x[i], xlab = xlab, ylab = ylab, type = type,
                              xlim = xlim, ylim = ylim, main = main[i],
-                              col = col[[i]], labels = labels,
+                              col = col[[i]], labels = labels[[i]],


Maybe the default for label in .plot_single_spectrum should be changed as well?
(and instead of testing label in each of the other function it should be done once in .plot_single_spectrum).

It had to come before because I needed to match the spectrum and the labels before .plot_single_spectrum is called. The default labels in .plot_single_spectrum thus still reamains a character.

jorainer

I'm OK merging after Sebastian's comments have been addressed.

This might now however break code in other packages too - could you, for backward compatibility, maybe you could add a check to the functions

if (is.character(labels))
    labels <- as.list(labels)

still, in the documentation etc I would not mention the support for character, this is purely for existing code in other packages or our analysis workflows that would now break, because we're passing a character.

guideflandre · 2025-04-04T08:55:52Z

This might now however break code in other packages too - could you, for backward compatibility, maybe you could add a check to the functions
if (is.character(labels))
    labels <- as.list(labels)

I changed into this:

if (is.character(labels))
    labels <- list(labels)

Since otherwise it would create a list of length equal to the number of peaks in the spectrum.

guideflandre · 2025-04-04T12:21:55Z

There was also an error with the old par not being returned after plotting multiple plots. I made sure plotSpectra now returns it

jorainer · 2025-04-07T06:57:14Z

what puzzles me a bit is that the GHA are not running for this PR. Would be nice to actually see if all unit tests run smoothly

guideflandre · 2025-04-07T09:42:44Z

what puzzles me a bit is that the GHA are not running for this PR. Would be nice to actually see if all unit tests run smoothly

I ran the tests test_plotting-functions.R and they all pass when I remove the old_par <- par(no.readonly = TRUE) & on.exit(par(old_par)).
I still believe this should be included. When I included them and compared the plots from the unit test, there are no visual changes whatsoever, only the par is different. When retaining the old par, it removes the annoyance of having a single plot be plotted in a [1,2] matrix after plotting 2 plots with plotSpectra. I could make a separate PR for this, but really it just removes this annoyance.

lgatto · 2025-04-07T13:15:51Z

Did you try to rebuild the vignettes and reverse dependencies?

I am trying to use the revdepcheck package, but so far without success - the supposed errors don't make any sense. Testing a couple of dependencies by hand works.

Vignettes also build locally.

lgatto · 2025-04-07T13:19:10Z

what puzzles me a bit is that the GHA are not running for this PR. Would be nice to actually see if all unit tests run smoothly

I have updated the action file - I think the checks should be run now for PRs.

lgatto · 2025-04-07T14:25:13Z

I am trying to use the revdepcheck package, but so far without success - the supposed errors don't make any sense.

It was apparently a timeout issue. All reverse dependencies are ok.

> revdep_summary()
✔ CompoundDb 1.11.2                      ── E: 0     | W: 0     | N: 0    
✔ hdxmsqc 1.3.0                          ── E: 0     | W: 0     | N: 0    
✔ koinar 1.1.2                           ── E: 0     | W: 0     | N: 0    
✔ MetaboAnnotation 1.11.1                ── E: 0     | W: 0     | N: 0    
✔ MetCirc 1.37.0                         ── E: 0     | W: 0     | N: 0    
✔ MetNet 1.25.0                          ── E: 0     | W: 0     | N: 0    
✔ MsBackendMassbank 1.15.3               ── E: 0     | W: 0     | N: 0    
✔ MsBackendMetaboLights 1.1.4            ── E: 0     | W: 0     | N: 0    
✔ MsBackendMgf 1.15.2                    ── E: 0     | W: 0     | N: 0    
✔ MsBackendMsp 1.11.1                    ── E: 0     | W: 0     | N: 0    
✔ MsBackendRawFileReader 1.13.1          ── E: 0     | W: 0     | N: 0    
✔ MsBackendSql 1.7.4                     ── E: 0     | W: 0     | N: 0    
✔ MsDataHub 1.7.3                        ── E: 0     | W: 0     | N: 0    
✔ MsExperiment 1.9.1                     ── E: 0     | W: 0     | N: 0    
✔ MSnbase 2.33.4                         ── E: 0     | W: 0     | N: 0    
✔ MsQuality 1.7.1                        ── E: 0     | W: 0     | N: 0    
✔ PSMatch 1.11.0                         ── E: 0     | W: 0     | N: 0    
✔ RaMS 1.4.3                             ── E: 0     | W: 0     | N: 0    
✔ SpectraQL 1.1.0                        ── E: 0     | W: 0     | N: 0    
✔ xcms 4.5.4                             ── E: 0     | W: 0     | N: 0

lgatto · 2025-04-07T20:06:42Z

@sgibb - I confirm that the package and all revdeps check correctly.

Update plotSpectra environment to allow addFragments returned list

c0ec9de

guideflandre mentioned this pull request Jan 22, 2025

Fix error when calling multiple Spectra objects in addFragments rformassspectrometry/PSMatch#20

Merged

jorainer requested changes Jan 23, 2025

View reviewed changes

Spectra.Rproj Outdated Show resolved Hide resolved

R/plotting-functions.R Show resolved Hide resolved

R/plotting-functions.R Outdated Show resolved Hide resolved

R/plotting-functions.R Show resolved Hide resolved

guideflandre and others added 4 commits January 23, 2025 15:04

Update based on PR remarks

945ab98

Remove unnecessare files Rproj and Rhistory

7caf05c

Delete Spectra.Rproj

4814b87

Delete .Rhistory

6487255

change labels argument to accept a list instead of a character in the…

6d701d9

… plotting functions

guideflandre closed this Mar 7, 2025

guideflandre force-pushed the main branch from 6d701d9 to bac7a3d Compare March 7, 2025 14:06

guideflandre added 2 commits March 7, 2025 15:38

Merge branch 'main' of https://github.com/guideflandre/Spectra

46f31e3

Update NEWS.md file

d6cabb9

guideflandre reopened this Mar 7, 2025

guideflandre added 3 commits March 7, 2025 16:00

Update examples

388384f

Update documentation

72c9e01

Update Vignette

97468e9

guideflandre requested a review from jorainer March 19, 2025 14:19

sgibb requested changes Apr 3, 2025

View reviewed changes

jorainer approved these changes Apr 3, 2025

View reviewed changes

Update plotSpectra based on review

965d84b

guideflandre added 2 commits April 4, 2025 13:43

update Documentation

b43ce92

Update plotSepctra to return old par

ca1a29c

update example

e3253cd

update the man

a84e82f

remove par correction for later PR

1f7b959

lgatto approved these changes Apr 7, 2025

View reviewed changes

lgatto merged commit c839f96 into rformassspectrometry:main Apr 7, 2025
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update plotSpectra environment to allow addFragments returned list #348

Update plotSpectra environment to allow addFragments returned list #348

guideflandre commented Jan 22, 2025

jorainer left a comment

jorainer commented Jan 23, 2025

guideflandre commented Jan 23, 2025

guideflandre commented Jan 23, 2025

lgatto commented Mar 6, 2025

sgibb left a comment

sgibb Apr 3, 2025

guideflandre Apr 4, 2025

sgibb Apr 3, 2025

guideflandre Apr 4, 2025

jorainer left a comment

guideflandre commented Apr 4, 2025

guideflandre commented Apr 4, 2025

jorainer commented Apr 7, 2025

guideflandre commented Apr 7, 2025

lgatto commented Apr 7, 2025 •

edited

Loading

lgatto commented Apr 7, 2025

lgatto commented Apr 7, 2025 •

edited

Loading

lgatto commented Apr 7, 2025

Update plotSpectra environment to allow addFragments returned list #348

Update plotSpectra environment to allow addFragments returned list #348

Conversation

guideflandre commented Jan 22, 2025

jorainer left a comment

Choose a reason for hiding this comment

jorainer commented Jan 23, 2025

guideflandre commented Jan 23, 2025

guideflandre commented Jan 23, 2025

lgatto commented Mar 6, 2025

sgibb left a comment

Choose a reason for hiding this comment

sgibb Apr 3, 2025

Choose a reason for hiding this comment

guideflandre Apr 4, 2025

Choose a reason for hiding this comment

sgibb Apr 3, 2025

Choose a reason for hiding this comment

guideflandre Apr 4, 2025

Choose a reason for hiding this comment

jorainer left a comment

Choose a reason for hiding this comment

guideflandre commented Apr 4, 2025

guideflandre commented Apr 4, 2025

jorainer commented Apr 7, 2025

guideflandre commented Apr 7, 2025

lgatto commented Apr 7, 2025 • edited Loading

lgatto commented Apr 7, 2025

lgatto commented Apr 7, 2025 • edited Loading

lgatto commented Apr 7, 2025

lgatto commented Apr 7, 2025 •

edited

Loading

lgatto commented Apr 7, 2025 •

edited

Loading